-
Notifications
You must be signed in to change notification settings - Fork 68
feat: context datastore #238
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
This change allowed us to save minutes( |
This is a very nice way to generically handle using batching and transaction functionality of an underlying database. This could replace the @lidel Now we need to decide if we want to support the generaic approach only and get rid of @Wondertan An alternative solution to your delete problem is to add a |
The DeleteMany wouldnt work outside of Blockstore. The blockstore is an example I gave to be familiar for shipyard folks, but its not exactly my case, so DeleteMany wouldnt be sufficient. |
I made a discovery that I am currently confused about. The performance improvement results from this change are wrong. The results are actually measured with hacky poc for this change. The final, supposed to be the production version in this PR, actually adds time overhead of ~20s compared to the baseline. |
False alarm. I triple checked, and there was a bug on the context datastore usage side, which made this change effectively a noop. After the fix, the results are on par with the hacky version I referenced earlier. |
Allows batch deleting across API boundaries. Depends on #320 Depends on ipfs/go-datastore#238, but we shouldn't block on it being released upstream. It should be fine to temporarily live with the replace so rollout pruning on time w/o: `{"from_height": 1, "to_height": 6500189, "took": 341.992917359}` w/: `{"from_height": 1, "to_height": 6500617, "took": 129.445211431}`
This is a modification to low-level interfaces that will require many potential changes. We need to take more time to consider the larger implications of this change. This is currently outside of the scope of our current maintenance workload agreed with the IPFS foundation in 2025, so we need to defer this until a later time. This can be discussed for inclusion in future dev grant work. |
@gammazero, I am a bit confused. How is this change may require changes? Its an independent feature that doesn't have any connections with any existing code in kubo/boxo/rainbow/libp2p/etc. It doesn't hurt to include this and then later on IPFS representatives can decide if they want to integrate this feature. Meanwhile, merging and releasing this would allow us to remove the ugly replace in go.mod across our repositories. If you are worry about maintenance overhead, I can reassure you that you can ping me and I will there to help with whatever issue there is with this code. However, I highly doubt there will be unless, you actually put efforts into integrating this. |
@Wondertan It is not that this change itself requires any changes, but about the potential consequences leading to changes. What I mean by this is that we would need to decide how to recommend that implementers use the new context datastore, particularly with other competing APIs like Here are some of the issues we have not had sufficient time to consider: High level
Regarding the PR it batches reads + writes but leaves commits out of scope. Is this what everyone wants / expects? Possible alternatives here look like:
Given your interest in this, I am going to put this back on our queue for reconsideration. However, I hope this gives a better explanation of why I was saying this was out of scope, at least for now. |
Hey @gammazero, appreciate the elaboration. Generally, I agree that there should be a single canonical way of approaching things, and in this case, having What I can say for sure is that the current set of options within the repo does not enable the optimization this PR targets. and simply adding My proposal is to extract the discussion on seeking the single canonical way for bathing things into a dedicated issue and unblock this PR. I am happy to add input on the use case we envision on our side. P.S. One of many things I've learned from IPFS codebases is how to structure dependency graph coherently, so that replaces are never needed. Please, don't force a replace on us 🙏, we are drowning in them already for orthogonal reasons. I had one confused user today who couldn't figure out where to get |
@aschmahmann We need to make a decision whether this is within the scope of code we want to maintain and support. We are currently not consuming this code ourselves (in kubo, boxo, rainbow, or IPFS cluster), so we need to consider our policy for accepting things like this into our codebase. Maybe we should consider turning this into a draft PR until there is something in our software that uses this. |
@aschmahmann, do you think we could bring clarity about next steps here soon? Should we assume that IPFS Shipyard no longer accepts external contributions? Please let us know. |
@Wondertan apologies for not getting to this earlier.
No, we accept contributions across the various repositories we maintain As for this PR in particular, my understanding is the objections to including it are:
All this being said you've been around the ecosystem enough that if you feel the objections above are unwarranted and this would be useful to people we're fine to take it and see how it plays out. Lmk if you'd still like us to merge and we'll do it. |
Yes, we would appreciate a merge here 🙏! Thank you @aschmahmann. If this datastore causes any form of issue or brings maintenance overhead, don't hesitate to ping me on GH and we will handle it. |
Batch and Txs allow grouping multiple writes or reads together to optimize speed or provide atomicity. However, there is no way to benefit from this grouping when you work across or outside the datastore interface boundaries. Example:
The context datastore solves the problem above, enabling the datastore underneath Blockstore to be wrapped. The wrapper then intercepts individual operations and reroutes them into the respective Read or Write. Example: